Fix the error catching logic of should().throw() in audit.js #12606

chromium-wpt-export-bot · 2018-08-21T22:43:55Z

Context: #12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

Change audit.js, audionodeoptions.js on both locations.
(wpt, non-wpt)
Update affected test files with the script.
Update the rest of test files which can't be updated
programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
Reviewed-on: https://chromium-review.googlesource.com/1184146
Commit-Queue: Hongchan Choi [email protected]
Reviewed-by: Raymond Toy [email protected]
Cr-Commit-Position: refs/heads/master@{#587660}

wpt-pr-bot

Already reviewed downstream.

karlt · 2018-08-22T05:52:25Z

@hoch, can you describe the exact situation, please, where the previous code
was causing a problem?

Is there a case where a test needs to accept either of a simple exception or a
DOMException? Wouldn't it be better to be specific about what is expected?

AIUI the intention of #11983 was
that tests would be specific about the kind of error that is thrown.

With this change, I don't see how a test could require that an exception is a
DOMException.

If the expected exception is a simple exception, then wouldn't it be better for
that test to require that the exception is a simple exception, by specifying
the constructor of the simple exception?

(It does look like the previous documentation "It can also be an instance of
either an Error or a DOMException" was incorrect, as the code required a
constructor and the DOMException constructor would not have a specific
name.)

hoch · 2018-08-22T15:25:25Z

should().throw() is an assertion to catch any kind of error. At least, that's why I made it.

Please consider this example:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/webaudio/audio-scheduled-source-basic.html

After the previous change (#11983), the test above started failing. If the assertion only expects to see DOMException, how can we catch things like TypeError or ReferenceError? rtoy@ and I thought generalize it to Error is okay, but feel free to let us know if you have something else in mind.

karlt · 2018-08-22T22:18:37Z

If the assertion only expects to see DOMException, how can we catch things like TypeError or ReferenceError?

The assertion would have only expected to see DOMException if expected was a string.
To catch simple exceptions like TypeError etc., should().throw(TypeError) was passed.

That catches bugs like https://bugzilla.mozilla.org/show_bug.cgi?id=1472095

/cc @padenot @bzbarsky

hoch · 2018-08-22T22:35:07Z

The previous change created ~10 test failures on our side. I don't have a strong opinion on this, but why do we want to care about DOMException specifically? IMO should().throw() should be generic enough to catch different types of errors or exceptions, and log its detail (type, name and etc) in the message.

karlt · 2018-08-22T23:01:21Z

The existing web platform tests were updated for the previous change. I would
have expected the only failures resulting from the changes to be in recent
tests that are yet to be upstreamed, unless audit.js from wpt is somehow being
used for tests outside wpt.

We care about DOMException specifically if the Web Audio API requires that a
DOMException is thrown.

If no parameters were passed to throw() then AFAIK it was still generic enough
to catch different types of exceptions.

If the Web Audio API requires that a simple exception such as TypeError is
thrown, then the framework was still generic enough to test also for that
specific exception.

Are you saying that there was a problem logging the detail of the failure?
Or do you not want to test the specific type of the exception?
Is there a reason why you only want to test the name and not the type?

hoch · 2018-08-23T17:12:58Z

We care about DOMException specifically if the Web Audio API requires that a
DOMException is thrown.

If no parameters were passed to throw() then AFAIK it was still generic enough
to catch different types of exceptions.

If we really want to specific about the type of error of exception, we should prevent the string argument in the assertion and enforce to use a specific type instead. Would it break many existing tests on your end? If not, I would like to remove the support on string type argument from throw() method.

I just did not have the context of this change, so this clarification really helped me understand. Please ignore what I said about "the detail of failure".

hoch · 2018-08-23T19:51:16Z

How about something like this?

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

Basically throw() will expect the name of exception with the first arg is DOMException.

karlt · 2018-08-23T21:48:05Z

That sounds fine to me, thank you. Mozilla is using audit.js with only
web-platform-tests and so there are no other tests that would need updating.

Just so you are aware, that would be different syntax from the
assert_throws() function which expects {object|number|string} for the
expected parameter. number or string mean to expect DOMExceptions.
object is for an argument like new TypeError().

wpt/resources/testharness.js

Line 1300 in f8ff0b7

* Assert an Exception with the expected code is thrown.

The current syntax is already somewhat different, and so this is not a big deal, but
this may be a good time to consider making the syntax more similar.

(The other thing I notice in assert_throws() is that it does not use
instanceof because that would fail when used on an exception from a different
window. To workaround that somewhat, names are limited to recognized names
and the code property is also tested. AFAIK web audio tests do not need to
care about different windows, and so I guess audit.js does not need to do
the same.

wpt/resources/testharness.js

Line 1417 in f8ff0b7

//We'd like to test that e instanceof the appropriate interface,

)

hoch · 2018-08-24T17:15:46Z

That is a good point. However, we actually created audit.js because we did not like much about what the test harness offers. So diverging from the harness is not a big deal from our perspective.

The latest patch I made (still working on WPT side) has this change and I think it's a bit clearer than we what had before.

Context: #12606 Change the logic of should.throw() so we can handle 3 cases - should(someExpression).throw(); should(someExpression).throw(TypeError); should(someExpression).throw(DOMException, 'NotSupportedError'); The generic error (except for DOMException) can be passed without the second argument, but this change will enforce the second arg when the expected error is a DOMException type. This touches many test files, so the work will be done in several steps: 1. Change audit.js on both locations (wpt, non-wpt) 2. Update affected test files with the script. 3. Update the rest of test files which can't be updated programmatically. Bug: 865614 Test: All layout tests pass. Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f

Context: #12606 Change the logic of should.throw() so we can handle 3 cases - should(someExpression).throw(); should(someExpression).throw(TypeError); should(someExpression).throw(DOMException, 'NotSupportedError'); The generic error (except for DOMException) can be passed without the second argument, but this change will enforce the second arg when the expected error is a DOMException type. This touches many test files, so the work will be done in several steps: 1. Change audit.js, audionodeoptions.js on both locations. (wpt, non-wpt) 2. Update affected test files with the script. 3. Update the rest of test files which can't be updated programmatically. Bug: 865614 Test: All layout tests pass. Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f

Context: web-platform-tests/wpt#12606 Change the logic of should.throw() so we can handle 3 cases - should(someExpression).throw(); should(someExpression).throw(TypeError); should(someExpression).throw(DOMException, 'NotSupportedError'); The generic error (except for DOMException) can be passed without the second argument, but this change will enforce the second arg when the expected error is a DOMException type. This touches many test files, so the work will be done in several steps: 1. Change audit.js, audionodeoptions.js on both locations. (wpt, non-wpt) 2. Update affected test files with the script. 3. Update the rest of test files which can't be updated programmatically. Bug: 865614 Test: All layout tests pass. Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f Reviewed-on: https://chromium-review.googlesource.com/1184146 Commit-Queue: Hongchan Choi <[email protected]> Reviewed-by: Raymond Toy <[email protected]> Cr-Commit-Position: refs/heads/master@{#587660}

Context: #12606 Change the logic of should.throw() so we can handle 3 cases - should(someExpression).throw(); should(someExpression).throw(TypeError); should(someExpression).throw(DOMException, 'NotSupportedError'); The generic error (except for DOMException) can be passed without the second argument, but this change will enforce the second arg when the expected error is a DOMException type. This touches many test files, so the work will be done in several steps: 1. Change audit.js, audionodeoptions.js on both locations. (wpt, non-wpt) 2. Update affected test files with the script. 3. Update the rest of test files which can't be updated programmatically. Bug: 865614 Test: All layout tests pass. Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f Reviewed-on: https://chromium-review.googlesource.com/1184146 Commit-Queue: Hongchan Choi <[email protected]> Reviewed-by: Raymond Toy <[email protected]> Cr-Commit-Position: refs/heads/master@{#587660}

foolip · 2018-08-31T14:13:45Z

This PR has not been merged automatically because the Firefox job is failing due to taking too long. (Travis timeout is 50 minutes.)

Suspecting that might be because the tests were made broken (timeout) I tested some of the affected tests before and after the changes but couldn't observe any difference. So this is just #7660 again, I think. I'll force merge.

This is a case where #7475 would have come in handy.

….throw() in audit.js, a=testonly Automatic update from web-platform-testsFix the error catching logic of should().throw() in audit.js Context: web-platform-tests/wpt#12606 Change the logic of should.throw() so we can handle 3 cases - should(someExpression).throw(); should(someExpression).throw(TypeError); should(someExpression).throw(DOMException, 'NotSupportedError'); The generic error (except for DOMException) can be passed without the second argument, but this change will enforce the second arg when the expected error is a DOMException type. This touches many test files, so the work will be done in several steps: 1. Change audit.js, audionodeoptions.js on both locations. (wpt, non-wpt) 2. Update affected test files with the script. 3. Update the rest of test files which can't be updated programmatically. Bug: 865614 Test: All layout tests pass. Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f Reviewed-on: https://chromium-review.googlesource.com/1184146 Commit-Queue: Hongchan Choi <[email protected]> Reviewed-by: Raymond Toy <[email protected]> Cr-Commit-Position: refs/heads/master@{#587660} -- wpt-commits: 4fd41de6ecf73b21534c81fc0f516defdca50ab1 wpt-pr: 12606

….throw() in audit.js, a=testonly Automatic update from web-platform-testsFix the error catching logic of should().throw() in audit.js Context: web-platform-tests/wpt#12606 Change the logic of should.throw() so we can handle 3 cases - should(someExpression).throw(); should(someExpression).throw(TypeError); should(someExpression).throw(DOMException, 'NotSupportedError'); The generic error (except for DOMException) can be passed without the second argument, but this change will enforce the second arg when the expected error is a DOMException type. This touches many test files, so the work will be done in several steps: 1. Change audit.js, audionodeoptions.js on both locations. (wpt, non-wpt) 2. Update affected test files with the script. 3. Update the rest of test files which can't be updated programmatically. Bug: 865614 Test: All layout tests pass. Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f Reviewed-on: https://chromium-review.googlesource.com/1184146 Commit-Queue: Hongchan Choi <hongchanchromium.org> Reviewed-by: Raymond Toy <rtoychromium.org> Cr-Commit-Position: refs/heads/master{#587660} -- wpt-commits: 4fd41de6ecf73b21534c81fc0f516defdca50ab1 wpt-pr: 12606 UltraBlame original commit: 650ffc22e4fef5b4d464b33a09663b47ceb620c5

chromium-wpt-export-bot added chromium-export do not merge yet labels Aug 21, 2018

wpt-pr-bot added webaudio wg-audio labels Aug 21, 2018

wpt-pr-bot approved these changes Aug 21, 2018

View reviewed changes

chromium-wpt-export-bot force-pushed the chromium-export-cl-1184146 branch from 1dbd6cd to f265cff Compare August 21, 2018 22:59

chromium-wpt-export-bot force-pushed the chromium-export-cl-1184146 branch from f265cff to d16f5e3 Compare August 23, 2018 21:23

chromium-wpt-export-bot force-pushed the chromium-export-cl-1184146 branch from d16f5e3 to 851a3b5 Compare August 24, 2018 20:33

chromium-wpt-export-bot changed the title ~~Update audit.js and fix error catching logic in should().throw()~~ Fix the error catching logic of should().throw() in audit.js Aug 24, 2018

chromium-wpt-export-bot force-pushed the chromium-export-cl-1184146 branch from 851a3b5 to b848489 Compare August 24, 2018 20:42

chromium-wpt-export-bot force-pushed the chromium-export-cl-1184146 branch from b848489 to e9a0b5e Compare August 24, 2018 22:10

chromium-wpt-export-bot force-pushed the chromium-export-cl-1184146 branch from e9a0b5e to 33048dc Compare August 24, 2018 22:19

chromium-wpt-export-bot force-pushed the chromium-export-cl-1184146 branch from 33048dc to cf2de7c Compare August 24, 2018 23:03

chromium-wpt-export-bot force-pushed the chromium-export-cl-1184146 branch from cf2de7c to f749c8a Compare August 30, 2018 17:26

chromium-wpt-export-bot removed the do not merge yet label Aug 30, 2018

chromium-wpt-export-bot force-pushed the chromium-export-cl-1184146 branch from f749c8a to 91a7ab3 Compare August 30, 2018 18:16

foolip merged commit 4fd41de into master Aug 31, 2018

foolip deleted the chromium-export-cl-1184146 branch August 31, 2018 14:13

foolip mentioned this pull request Aug 31, 2018

What is webaudio/resources/audit.js? #12781

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the error catching logic of should().throw() in audit.js #12606

Fix the error catching logic of should().throw() in audit.js #12606

chromium-wpt-export-bot commented Aug 21, 2018 •

edited

Loading

wpt-pr-bot left a comment

karlt commented Aug 22, 2018

hoch commented Aug 22, 2018

karlt commented Aug 22, 2018

hoch commented Aug 22, 2018

karlt commented Aug 22, 2018

hoch commented Aug 23, 2018

hoch commented Aug 23, 2018

karlt commented Aug 23, 2018

hoch commented Aug 24, 2018

foolip commented Aug 31, 2018

Fix the error catching logic of should().throw() in audit.js #12606

Fix the error catching logic of should().throw() in audit.js #12606

Conversation

chromium-wpt-export-bot commented Aug 21, 2018 • edited Loading

wpt-pr-bot left a comment

Choose a reason for hiding this comment

karlt commented Aug 22, 2018

hoch commented Aug 22, 2018

karlt commented Aug 22, 2018

hoch commented Aug 22, 2018

karlt commented Aug 22, 2018

hoch commented Aug 23, 2018

hoch commented Aug 23, 2018

karlt commented Aug 23, 2018

hoch commented Aug 24, 2018

foolip commented Aug 31, 2018

chromium-wpt-export-bot commented Aug 21, 2018 •

edited

Loading